refactor(plugins): plugin system overhaul — phases 1-4 + 6 integrated - #428

app/src/plugins/settings/parameter-select.ts (1)

14-20: Add range consistency validation (max >= min, step > 0).
The schema currently permits invalid range configs that can break parameter rendering logic.

Suggested refine

 export const parameterSelectSettingsSchema = z
   .object({
@@
     rangeMin: z.coerce.number().default(0),
     rangeMax: z.coerce.number().default(100),
-    rangeStep: z.coerce.number().default(1),
+    rangeStep: z.coerce.number().positive().default(1),
@@
   })
+  .refine((v) => v.rangeMax >= v.rangeMin, {
+    message: "rangeMax must be greater than or equal to rangeMin",
+    path: ["rangeMax"],
+  })
   .passthrough();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/settings/parameter-select.ts` around lines 14 - 20, Add
validation to the parameter schema to enforce range consistency: ensure rangeMax
>= rangeMin and rangeStep > 0. Update the Zod schema that defines rangeMin,
rangeMax, and rangeStep (the object using z.coerce.number().default(...)) to
include a .refine() or .superRefine() on the schema to check these conditions
and return descriptive errors for the fields (e.g., referencing
rangeMin/rangeMax/rangeStep) so invalid configs are rejected at validation time.

app/src/plugins/settings/line.ts (1)

15-15: Constrain lineWidth to valid finite positive values.
Current coercion accepts values that can break rendering semantics (e.g., negative or non-finite). Add bounds.

Suggested constraint

-    lineWidth: z.coerce.number().default(2),
+    lineWidth: z.coerce.number().finite().positive().default(2),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/settings/line.ts` at line 15, The current schema lineWidth:
z.coerce.number().default(2) allows negative, zero, or non-finite values; update
the schema to enforce finite, positive bounds (e.g., replace with
z.coerce.number().finite().positive().default(2) or
z.coerce.number().finite().min( Number.EPSILON ).default(2)) so only valid
positive finite widths are accepted while keeping the default of 2; change the
expression containing lineWidth accordingly.

app/src/plugins/settings/iframe.ts (1)

8-12: Tighten iframe setting validation for URL and sandbox.
z.string().optional() allows malformed URLs and arbitrary sandbox tokens. Consider validating URL format and constraining sandbox tokens to known values to prevent invalid/surprising runtime behavior.

Suggested schema hardening

 export const iframeSettingsSchema = z
   .object({
-    url: z.string().optional(),
+    url: z.string().url().optional(),
     iframeTitle: z.string().optional(),
-    sandbox: z.string().optional(),
+    sandbox: z
+      .string()
+      .regex(
+        /^(allow-forms|allow-modals|allow-orientation-lock|allow-pointer-lock|allow-popups|allow-popups-to-escape-sandbox|allow-presentation|allow-same-origin|allow-scripts|allow-top-navigation|allow-top-navigation-by-user-activation)(\s+(allow-forms|allow-modals|allow-orientation-lock|allow-pointer-lock|allow-popups|allow-popups-to-escape-sandbox|allow-presentation|allow-same-origin|allow-scripts|allow-top-navigation|allow-top-navigation-by-user-activation))*$/,
+      )
+      .optional(),
   })
   .passthrough();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/settings/iframe.ts` around lines 8 - 12, The current schema
leaves url and sandbox too loose: replace the url field's z.string().optional()
with z.string().url().optional() (or z.string().refine(...) if you need custom
checks) to enforce valid URL format, and restrict sandbox by replacing
z.string().optional() with either z.enum([...]).optional() for single known
tokens or z.string().optional().refine(val => val.split(/\s+/).every(t =>
allowedSandboxTokens.has(t))) where allowedSandboxTokens is the set of standard
iframe sandbox tokens (e.g., allow-forms, allow-modals, allow-pointer-lock,
allow-popups, allow-popups-to-escape-sandbox, allow-presentation,
allow-same-origin, allow-scripts, allow-storage-access-by-user-activation); keep
iframeTitle as-is or add z.string().min(1).optional() if empty titles should be
rejected. Ensure you update the schema that defines these fields (the entries
named url, iframeTitle, sandbox) using zod validators mentioned above.

app/src/components/widget-editor-modal.tsx (1)

404-408: Type cast may be unnecessary.

If getAllChartTypes() already returns ChartType[] (or readonly ChartType[]), the cast as ChartType[] is redundant. If it returns string[], consider updating getAllChartTypes return type instead of casting at call sites.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/components/widget-editor-modal.tsx` around lines 404 - 408, The code
is using a redundant type cast on getAllChartTypes() in the selected chart types
memo block; either remove the unnecessary "as ChartType[]" cast where
getAllChartTypes() is used in widget-editor-modal (alongside
getCompatibleChartTypes and selectedConnection), or update the
getAllChartTypes() function signature to return ChartType[] (or readonly
ChartType[]) so callers don't need casts—pick one: if the function already
returns ChartType[], remove the cast; if it returns string[], change its return
type to ChartType[] and adjust its implementation accordingly.

app/src/components/__tests__/card-container.test.tsx (1)

256-268: Stale comment — test doesn't exercise form widget.

The comment on line 258 says "Need to add 'form' to the mock chart-helpers" but the test uses chartType: "bar", which is already mocked. Either update the comment to match the test intent or change the test to actually exercise the form path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/components/__tests__/card-container.test.tsx` around lines 256 - 268,
The inline comment in the test "renders chart for form widgets without querying"
is stale: it suggests adding "form" to the mock chart-helpers but the test
creates a widget via createWidget with chartType: "bar". Update the test to be
consistent by either (A) changing the comment to reflect that this test covers a
bar chart path, or (B) if you intend to exercise the form widget path, change
the widget creation in this test to use chartType: "form" (and adjust any
mock/setup for chart-helpers accordingly) so the CardContainer render with
widget={widget} and previewData triggers the form-specific code path.

app/src/plugins/index.ts (1)

65-75: Consider bidirectional validation.

The current check warns when CHART_TYPES entries lack a plugin, but doesn't catch plugins registered without a corresponding CHART_TYPES entry. If the intent is strict synchronization, consider also checking the inverse:

for (const t of pluginRegistry.getTypes()) {
  if (!CHART_TYPES.includes(t as typeof CHART_TYPES[number])) {
    console.warn(`Plugin "${t}" registered but not in CHART_TYPES`);
  }
}

This would catch orphaned plugins. If external/dynamic plugins are expected, the current one-way check is fine.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/index.ts` around lines 65 - 75, Add a reverse validation to
ensure no plugin is registered without a corresponding CHART_TYPES entry: after
computing registeredTypes (from pluginRegistry.getTypes()) iterate over
pluginRegistry.getTypes() and warn when a type is not included in CHART_TYPES
(use CHART_TYPES.includes to check). Keep the original one-way check and add
this inverse loop to catch orphaned plugins (log with a clear message like
`Plugin "${t}" registered but not in CHART_TYPES"`).

app/src/plugins/bar.tsx (1)

30-30: Chart error boundary already handles parse failures gracefully.

The error boundary at ChartRenderer catches render errors and displays a fallback UI instead of crashing the dashboard. While barSettingsSchema.parse(raw) can still throw on invalid settings, using safeParse with defaults would be a better UX — allowing the widget to render with safe defaults instead of showing an error state.

Apply this refactor across all plugin render paths for consistency:

Suggested improvement

-  const settings = barSettingsSchema.parse(raw);
+  const parsed = barSettingsSchema.safeParse(raw);
+  const settings = parsed.success ? parsed.data : barSettingsSchema.parse({});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/bar.tsx` at line 30, Replace the direct parsing call so
invalid settings don't throw: instead of barSettingsSchema.parse(raw) use
barSettingsSchema.safeParse(raw) and, when safeParse returns success === false,
fall back to a defaults object (or merged defaults) so the widget renders with
safe defaults; update the render path that calls barSettingsSchema.parse(raw)
and mirror the same safeParse+defaults pattern across other plugin renderers
(e.g., any code invoked by ChartRenderer) so parsing failures are handled
gracefully rather than letting parse throw.

app/src/lib/chart-helpers.ts (1)

27-31: Consider deriving column mapping support from plugin capabilities.

The hardcoded COLUMN_MAPPING_TYPES Set requires manual updates when adding charts that support column mapping. Consider adding a supportsColumnMapping capability flag to plugin definitions in a future phase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/chart-helpers.ts` around lines 27 - 31, COLUMN_MAPPING_TYPES is
currently a hardcoded Set limiting which charts support column mapping; instead,
update chart-helpers.ts to derive supported chart types from the plugin registry
by checking a new supportsColumnMapping flag on each plugin definition (e.g.,
plugins.map(p => p.type) where p.supportsColumnMapping === true) and build
COLUMN_MAPPING_TYPES from that dynamic list; modify any consumers of
COLUMN_MAPPING_TYPES to import the built set or a helper function like
getColumnMappingTypes() so future plugins simply declare supportsColumnMapping
without changing this module.

app/src/plugins/settings/__tests__/settings-schemas.test.ts (1)

414-446: Consider adding explicit field validation for form and table schemas.

Both formSettingsSchema and tableSettingsSchema tests rely entirely on passthrough behavior. If these schemas are intentionally minimal, this is fine. However, if specific fields like fields, submitLabel, pageSize, or showRowNumbers should be typed/validated, consider adding schema-level definitions and corresponding tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/settings/__tests__/settings-schemas.test.ts` around lines 414
- 446, Tests for formSettingsSchema and tableSettingsSchema only verify
passthrough behavior; if fields like fields, submitLabel, pageSize, or
showRowNumbers should be validated, update the schemas (formSettingsSchema and
tableSettingsSchema) to define those properties (e.g., fields as array of
objects with name/type, submitLabel as string, pageSize as number,
showRowNumbers as boolean) instead of passthrough, and add corresponding unit
tests that assert valid inputs are accepted and invalid values are rejected
(e.g., missing required keys, wrong types) to ensure schema-level validation
covers these fields.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/components/card-container.tsx (1)

38-40: Decouple column-mapping capability from hard-coded set

Line 40 relies on chartSupportsColumnMapping(type) backed by a hard-coded set ["bar", "line", "pie"] in app/src/lib/chart-helpers.ts. No plugins currently declare supportsColumnMapping: true, so no drift exists today. However, the set is not registry-driven—future plugins declaring the capability will silently fail to work unless the set is manually updated. Consider deriving this from plugin registrations to prevent silent capability misses.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/components/card-container.tsx` around lines 38 - 40, The
supportsColumnMapping function currently proxies to
chartSupportsColumnMapping(type) which uses a hard-coded set; update
supportsColumnMapping to consult the plugin registry/manifest for the chart
type’s declared capability (e.g., check each registered plugin or
pluginRegistry/PluginManifest entry for supportsColumnMapping: true for that
chart type) and fall back to chartSupportsColumnMapping(type) if no registration
exists, so future plugins that declare supportsColumnMapping are honored without
updating the hard-coded list.

app/src/lib/chart-plugin-registry.ts (1)

102-103: ⚠️ Potential issue | 🟡 Minor

Avoid possible implicit any in settingsSchema typing.

Line 103 sets only the first ZodType generic. In some Zod versions, omitted generics can still fall back to any. Please lock all generics explicitly to preserve strict typing.

♻️ Proposed change

-  settingsSchema?: z.ZodType<Record<string, unknown>>;
+  settingsSchema?: z.ZodType<
+    Record<string, unknown>,
+    z.ZodTypeDef,
+    Record<string, unknown>
+  >;

For the exact Zod version used in this repo, what are the default generic parameters of `ZodType`, and does `ZodType<Record<string, unknown>>` leave any generic as `any`?

As per coding guidelines, "**/*.{ts,tsx}: TypeScript must be strict. No any without a comment explaining why."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/chart-plugin-registry.ts` around lines 102 - 103, The
settingsSchema property currently uses a single ZodType generic which may leave
other ZodType generic parameters as implicit any; update the type annotation to
explicitly specify all generics (e.g. use z.ZodType<Record<string, unknown>,
z.ZodTypeDef, Record<string, unknown>>) so both the output and input types and
the Definition type are locked down for settingsSchema (reference symbol:
settingsSchema, type: z.ZodType).

app/src/plugins/single-value.tsx (1)

36-40: Cache default settings once instead of parsing on each render.

Line 39 runs singleValueSettingsSchema.parse({}) on every render when parse fails. Move this default parse to module scope and reuse it for a cheaper, cleaner fallback path.

Proposed refactor

+const defaultSingleValueSettings = singleValueSettingsSchema.parse({});
+
 function SingleValuePluginComponent({
   data,
   settings: raw,
@@
   const parsed = singleValueSettingsSchema.safeParse(raw);
   const settings = parsed.success
     ? parsed.data
-    : singleValueSettingsSchema.parse({});
+    : defaultSingleValueSettings;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/plugins/single-value.tsx` around lines 36 - 40, The code calls
singleValueSettingsSchema.parse({}) on every render as a fallback; create a
module-scope constant (e.g., DEFAULT_SINGLE_VALUE_SETTINGS) initialized once by
calling singleValueSettingsSchema.parse({}) and then change the fallback in the
component to use that constant instead of invoking parse({}) repeatedly (update
the settings assignment that currently references parsed/
singleValueSettingsSchema.parse({}) to use DEFAULT_SINGLE_VALUE_SETTINGS).

app/src/lib/__tests__/chart-helpers.test.ts (3)

209-216: Type count assertion is good but brittle.

Hard-coded 17 will fail if chart types are added/removed. The loop checking CHART_TYPES inclusion is the more valuable assertion.

♻️ Alternative: derive count from CHART_TYPES

 describe("getAllChartTypes", () => {
   it("returns all 17 registered types", () => {
     const types = getAllChartTypes();
-    expect(types.length).toBe(17);
+    expect(types.length).toBe(CHART_TYPES.length);
     for (const t of CHART_TYPES) {
       expect(types).toContain(t);
     }
   });
 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 209 - 216, The test
for getAllChartTypes uses a brittle hard-coded 17; change the count assertion to
derive the expected value from CHART_TYPES (e.g., compare types.length to
CHART_TYPES.length) and keep the existing loop that ensures every CHART_TYPES
entry is present; update the assertion that references the literal 17 to use
CHART_TYPES.length so the test won't break when chart types are added or
removed.

---

184-188: getChartDefaults test coverage is minimal.

Only tests bar returning an empty object. If any chart type has non-trivial defaults in the future, consider adding more cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 184 - 188, The test
for getChartDefaults only asserts the "bar" case returns {}, so expand coverage
by adding additional assertions for other chart types (e.g., "line", "pie",
"scatter" or any types referenced in getChartDefaults) to ensure any non-trivial
defaults are validated; update the test in chart-helpers.test.ts to include
these cases (preferably table-driven/parameterized tests) and assert the
expected default objects returned by getChartDefaults for each chart type.

---

25-39: Simplify the next/dynamic mock.

The mock works but is more elaborate than necessary. The try/catch and Promise-checking logic always returns Stub regardless of the outcome. A simpler mock achieves the same:

♻️ Simplified mock

 vi.mock("next/dynamic", () => ({
-  default: (fn: () => Promise<{ default: unknown }>) => {
-    try {
-      const mod = fn();
-      if (
-        mod &&
-        typeof (mod as Promise<{ default: unknown }>).then === "function"
-      )
-        return Stub;
-    } catch {
-      /* noop */
-    }
-    return Stub;
-  },
+  default: () => Stub,
 }));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/__tests__/chart-helpers.test.ts` around lines 25 - 39, The
next/dynamic mock is overly complex and always returns Stub; simplify by
replacing the current implementation in the vi.mock("next/dynamic", ...) block
so its default export directly returns Stub (remove the try/catch and
Promise-checking logic). Locate the mock where default is defined and change it
to a minimal stub-returning implementation referencing the existing Stub symbol
so tests remain identical but code is clearer.